Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

@XmlElement notation consistency #219

Merged
merged 1 commit into from
Jan 22, 2014
Merged

@XmlElement notation consistency #219

merged 1 commit into from
Jan 22, 2014

Conversation

ajgarlag
Copy link
Contributor

When I opened the PR #58 there was no @XmlElement annotation defined, but the @XmlElement annotation was introduced in #187, before #58 was merged.

Now the @XmlElement has two properties: cdata and namespace which have different notations in Yaml and Xml mappings.

I think this could be fixed in order to improve consistency. What do you think about?

Yaml

Current notation

JMS\Serializer\Tests\Fixtures\BlogPost:
    properties:
        author:
            type: JMS\Serializer\Tests\Fixtures\Author
            groups: [post]
            xml_namespace: http://www.w3.org/2005/Atom
            xml_element:
                cdata: false

Proposed notations

Option 1

JMS\Serializer\Tests\Fixtures\BlogPost:
    properties:
        author:
            type: JMS\Serializer\Tests\Fixtures\Author
            groups: [post]
            xml_namespace: http://www.w3.org/2005/Atom
            xml_cdata: false

Option 2

JMS\Serializer\Tests\Fixtures\BlogPost:
    properties:
        author:
            type: JMS\Serializer\Tests\Fixtures\Author
            groups: [post]
            xml_element:
                namespace: http://www.w3.org/2005/Atom
                cdata: false

XML

Current notation

<serializer>
    <class name="JMS\Serializer\Tests\Fixtures\BlogPost" xml-root-name="blog-post">
        <property name="author" groups="post" type="JMS\Serializer\Tests\Fixtures\Author" xml-namespace="http://www.w3.org/2005/Atom">
            <xml-element cdata="false"/>
        </property>
    </class>
</serializer>

Proposed notations

Option 1

<serializer>
    <class name="JMS\Serializer\Tests\Fixtures\BlogPost" xml-root-name="blog-post">
        <property name="author" groups="post" type="JMS\Serializer\Tests\Fixtures\Author" xml-namespace="http://www.w3.org/2005/Atom" xml-cdata="false"/>
    </class>
</serializer>

Option 2

<serializer>
    <class name="JMS\Serializer\Tests\Fixtures\BlogPost" xml-root-name="blog-post">
        <property name="author" groups="post" type="JMS\Serializer\Tests\Fixtures\Author">
            <xml-element cdata="false" namespace="http://www.w3.org/2005/Atom"/>
        </property>
    </class>
</serializer>

@schmittjoh
Copy link
Owner

Sounds good.

I lean more towards option 2) as namespace is a newer addition, so it hopefully should not hurt too many people if we make this change.

@ajgarlag
Copy link
Contributor Author

Ok, I will start to work on this so it could be merged before you tag a new version.

@ajgarlag ajgarlag mentioned this pull request Jan 22, 2014
Merged
@mvrhov
Copy link

mvrhov commented Jan 22, 2014

IMO what @schmittjoh said about option 2 was plan all along, we talked about this when I was implementing that feature.

@ajgarlag
Copy link
Contributor Author

@schmittjoh I think is finished. Could you review it?

schmittjoh added a commit that referenced this pull request Jan 22, 2014
@schmittjoh
Merge pull request #219 from ajgarlag/xml-element-notation-consistency
67cf0f4 
@xmlelement notation consistency
@schmittjoh schmittjoh merged commit 67cf0f4 into schmittjoh:master Jan 22, 2014
@schmittjoh
Copy link
Owner

Thanks!

@ajgarlag
Copy link
Contributor Author

@schmittjoh Remember to update the documentation site if the update is not triggered automatically.

@schmittjoh
Copy link
Owner

There is a cronjob which updates this regularly. Just verified, the changes
are now live.

On Wed, Jan 22, 2014 at 1:23 PM, Antonio J. García Lagar <
[email protected]> wrote:

@schmittjoh https://github.com/schmittjoh Remember to update the
documentation site if the update is not triggered automatically.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/219#issuecomment-33016960
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ajgarlag @schmittjoh @mvrhov